Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Tooltip] Refactor touch handling #23088

Closed
wants to merge 1 commit into from

Conversation

eps1lon
Copy link
Member

@eps1lon eps1lon commented Oct 16, 2020

detectTouch was introduced in #20807. The added test still passes with this change so either the test is insufficient or the observed event order never actually happens (e.g. when emulating devices the events are sometimes wrong especially with Firefox).

@eps1lon eps1lon added the component: tooltip This is the name of the generic UI component, not the React module! label Oct 16, 2020
@mui-pr-bot
Copy link

Details of bundle changes

Generated by 🚫 dangerJS against 216fac6

@oliviertassinari
Copy link
Member

oliviertassinari commented Oct 16, 2020

I have noticed a change of behavior after click the "Click" button on iOS Safari 14 and Android Chrome 86. The tooltip is no longer rendered with the "touch" styles.

https://next.material-ui.com/components/tooltips/#triggers OK ✅

Capture d’écran 2020-10-16 à 12 45 12


https://deploy-preview-23088--material-ui.netlify.app/components/tooltips/#triggers KO ❌

Capture d’écran 2020-10-16 à 12 45 08

It sounds like we need to keep the logic but add a regression test case.

@eps1lon

This comment has been minimized.

@oliviertassinari

This comment has been minimized.

@eps1lon

This comment has been minimized.

@oliviertassinari

This comment has been minimized.

@eps1lon

This comment has been minimized.

@oliviertassinari

This comment has been minimized.

@eps1lon
Copy link
Member Author

eps1lon commented Oct 16, 2020

Thanks for the report 👍 That's definitely indicative of an actual issue but can we talk about <Tooltip disableFocusListener disableHoverListener disableTouchListener title="Add" />? What would be the point of only showing the tooltip after you clicked? Especially because that behavior needs another component (ClickAwayListener) to work.

@eps1lon
Copy link
Member Author

eps1lon commented Oct 16, 2020

Which goes right into the discussion of classes.tooltip:

Styles applied to the tooltip (label wrapper) element if the tooltip is opened by touch.

It's not actually opened by touch but click in the problematic demo so it's technically correct to not apply the styles. If you want to argue that this is concerned about "user-touch" then it doesn't make sense to apply these styles if disableTouchListener is set. Seems pretty confusing to me to use "touch" in both names but mean something different (user-touch vs touch-event).

@oliviertassinari
Copy link
Member

oliviertassinari commented Oct 16, 2020

Styles applied to the tooltip (label wrapper) element if the tooltip is opened by touch.

We could improve the description of this prop and the behavior. I see two concerns.

First, on a touch interaction, you want to compensate for the size of the finger that triggered it. Otherwise, you might not see the message, hidden behind your finger. It's the same concern with a mouse, only that it cover a smaller area (unless maginifed).
Second, on a smaller screen, we probably want to increase the size of the tooltip to improve readability.

So far, we have mixed the too and used the touch start event to trigger the style. I'm not aware of any significance issue with such approximation.

@oliviertassinari
Copy link
Member

oliviertassinari commented Oct 16, 2020

I'm not sure what's the right thing to do here. At least, I hope that the previous comment explain the objective with the style.

I can't remember why we have a disableTouchListener prop, I could only find #20245

@eps1lon
Copy link
Member Author

eps1lon commented Oct 16, 2020

At least, I hope that the previous comment explain the objective with the style.

I definitely understand why we have separate styles for touch devices. I think they should be applied at another point though. Why make the tooltip bigger when we register touch input but not the button that triggers it? Intuitively I rather add @media (pointer: coarse) { .Mui-Tooltip { font-size: 1.2rem } }

My point is more that if I want to disable the touch listener I would also expect any special handling of touch events to be ignored.

@oliviertassinari
Copy link
Member

oliviertassinari commented Oct 16, 2020

@media (pointer: coarse)

To be careful, it might not work on Samsung Galaxy phones, to be tested. We discovered that in #1500. In mui/material-ui-pickers#1653, I have found that @media (pointer: fine) was reliable.

My point is more that if I want to disable the touch listener I would also expect any special handling of touch events to be ignored.

Agree, for controlling the open state. I think that It would be surprising if the disableTouchListener prop impacts the style of the tooltip.

@eps1lon
Copy link
Member Author

eps1lon commented Oct 16, 2020

I think that It would be surprising if the disableTouchListener prop impacts the style of the tooltip.

I want to do some more interaction research but I'm fairly certain we can just drop this prop. enterTouchDelay={Number.POSITIVE_INFINITY} does the same. As it stands right now I'd rather make this behavior stand out in the implementation. disableTouchListener looks harmless when it is in fact not harmless at all.

@eps1lon
Copy link
Member Author

eps1lon commented Oct 16, 2020

Will revisit later.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: tooltip This is the name of the generic UI component, not the React module!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants